Skip to content

Conversation

@arshidkv12
Copy link
Contributor

No description provided.

@samsonasik
Copy link
Member

samsonasik commented Sep 17, 2025

This needs map opposite fixtures on rector-src on PR:

so we have similar functionality

Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs map opposite fixtures on rector-src on PR:

so we have similar functionality

@arshidkv12
Copy link
Contributor Author

arshidkv12 commented Sep 17, 2025

$isMatch = str_contains(mb_substr('abc', $offset), 'a');

Convert to

 $isMatch = mb_strpos('abc', 'a', $offset) !== false;

https://github.com/rectorphp/rector-src/blob/main/rules-tests/Php80/Rector/NotIdentical/MbStrContainsRector/Fixture/offset_variable_zero_strpos.php.inc

Am I right?

@samsonasik
Copy link
Member

Yes, ensure the value is match with upgrade code, the similar logic/service can be used.

{
public function run()
{
$isMatch = str_contains('😊abc', 'a');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking if the mb_strpos() only when needle is exact string and has multibyte, the original str_contains() to strpos() already converted on DowngradeStrContainsRector, so should be a logic to verify that the needle has multibye, eg:

strlen($needle) !== mb_strlen($needle)

this, however, I prefer to not be part of downgrade php 8.0 set, as dealing with multibyte can be "there on purpose", user that needs exact change str_contains -> mb_strpos needs to enable manually as use and know the risk.

{
public function run()
{
return str_contains('abc', 'a');
Copy link
Member

@samsonasik samsonasik Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only when needle has multibyte value as well:

Suggested change
return str_contains('abc', 'a');
return str_contains('😊abc', '😊a');

{
public function run()
{
return mb_strpos('abc', 'a') !== false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only when needle has multibyte value as well:

Suggested change
return mb_strpos('abc', 'a') !== false;
return mb_strpos('😊abc', '😊a') !== false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

{
public function run()
{
$isMatch = !str_contains('abc', 'a');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these fixtures are invalid, as no multibyte in needle, only downgrade to mb_strpos when needle has multibyte char

$isMatch = !str_contains('😊abc', 😊'a');

DowngradePropertyPromotionRector::class,
DowngradeNonCapturingCatchesRector::class,
DowngradeStrContainsRector::class,
DowngradeMbStrContainsRector::class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unregister this, there is no "mb_str_contains", only str_contains, that means that utilize strpos can be on purpose.

let user manually use if needed, dealing with user that may don't have mbstring extension or use strpos on purpose :)

user that use mb_ functions know why they are using the mb_ functions :)

if (! $funcCall instanceof FuncCall) {
return null;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

early skip first class callable here:

if ($funcCall->isFirstClassCallable()) {
    return null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!$funcCall->isFirstClassCallable()) {
  return null;
}

Correct ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, should be below if (! $funcCall instanceof FuncCall) {:

        if (! $funcCall instanceof FuncCall) {
            return null;
        }

        if ($funcCall->isFirstClassCallable()) {
            return null;
        }

{
public function run()
{
$isMatch = str_contains('😊abc', '😊a');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test fixture to skip no multibyte needle:

$isMatch = str_contains('abc', 'a');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the string without multibyte needle.

if (! $this->isName($haystack->name, 'mb_substr')) {
return null;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip first class callable haystack as well.

*
* @see \Rector\Tests\DowngradePhp80\Rector\FuncCall\DowngradeMbStrContainsRector\DowngradeMbStrContainsRectorTest
*/
final class DowngradeMbStrContainsRector extends AbstractRector
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to rename to something like: DowngradeStrContainsWithMultibyteNeedleRector since there is no mb_str_contains, make clearer it actually use when needle has multibyte.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@samsonasik
Copy link
Member

After some checks, this rule is not needed, strpos is fine, since it doesn't verify position, only check non-falsy result, so strpos was fine as is, even needle contains multibyte, see

https://3v4l.org/cc4Di

I am closing this as that reasoning. Thank you for understanding.

@samsonasik samsonasik closed this Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants